[MOD-14956] Add SQ8 quantization support for HNSW index - #1007
Conversation
Cherry-picked from ARM-software#4 (head 125ea15), squashing the fork's four commits into one. Adds 8-bit scalar quantization (SQ8) to the standalone HNSW index: * `VecSimQuantType` plus `quantType` / `quantParams` on `HNSWParams`. Both fields are appended at the end of the struct and `VecSimQuant_NONE` is 0, so existing zero-initialized and designated-initializer construction is unaffected. * `HNSWFactory` can build SQ8 indexes for FLOAT32 and FLOAT16 data types with the L2 and IP metrics, wiring `QuantPreprocessor` and `DistanceCalculatorWithNorm`, and accounts for SQ8 in `EstimateInitialSize` and `EstimateElementSize`. * For SQ8, `quantParams` points to a `float[dim]` mean vector; a null pointer selects quantization without mean normalization. * New `test_hnsw_sq8` unit-test target and suite. SQ8 support for the tiered HNSW index, serialization and benchmarks is deferred to later PRs in the MOD-14956 series. Redis-side adjustments made during the cherry-pick: * Dropped the added `SPDX-FileCopyrightText` Arm line from the two modified files, matching how #999, #1000 and #1002 landed. It is kept on the new `tests/unit/test_hnsw_sq8.cpp`, where the `BSD-3-Clause` identifier was replaced by this repo's Redis tri-license header. * Wrapped that header so `make check-format` passes at the 100-column limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5e84a59 to
2d29bd9
Compare
Follow-up review pass over the cherry-picked MOD-14956 change. No behavioural change is intended: all of these are interface, single-source-of-truth and idiom fixes. Public API (`vec_sim_common.h`): * `quantParams` is now `const void *`. Every use in the tree reads it, and two already cast it to `const float *`. The layout is unchanged, so this is not an ABI break, and callers passing a non-const pointer still compile. Worth doing now, before the field ships and freezes. * The `VecSimQuant_SQ8` comment claimed "with mean normalization". Mean normalization is optional and selected by `quantParams`, exactly as the field's own comment says. Reworded. Storage layout (`types/sq8.h`, `spaces/computer/preprocessors.h`, `index_factories/hnsw_factory.cpp`): * `GetSQ8StoredDataSize` re-derived the stored blob size that `QuantPreprocessor`'s constructors already computed. Two independent formulas for one layout drift silently, which is the bug class fixed in MOD-15303. The formula now lives once, as `sq8::storage_bytes_count<Metric, WithNorm>(dim)`, next to the `storage_metadata_count` it builds on, and both the preprocessor and the factory call it. Factory (`index_factories/hnsw_factory.cpp`): * Restored the `return NULL` that closes the SQ8 branch. It is unreachable today, since the type and metric checks leave only FP32/FP16 x L2/IP, but without it adding a type or metric silently falls through and builds an unquantized index. * `assert(ret == 0)` on `addPreprocessor` is now `assert(ret != -1)`. The function returns -1 on failure, 0 when the container is full, and the next free index otherwise, so 0 is merely the only success value at the current container size of one. `!= -1` is the documented contract and the existing repo idiom. * Hoisted the tail the two branches duplicated (container construction, `addPreprocessor`, assert, `IndexComponents`, return). Only the preprocessor and the distance calculator actually differ. * The mean vector is copied with a single `assign` instead of a zero-filling constructor followed by `memcpy`, which wrote every element twice. * Obtaining the query alignment required calling `GetDistFunc` for a function that is never used, since spaces.h offers no alignment-only query and the asymmetric hint covers the storage operand. That call now lives in a small `GetQueryAlignment<DataType>` adapter that returns the hint, so the call site neither discards a value nor keeps a third distance function in scope next to `sym_func` and `asym_func` that must never be called. `query_alignment` is const. * `GetSQ8StoredDataSize` is `[[nodiscard]] constexpr` and `dim` / `with_norm` are const. Verified: - ./check-format.sh - g++ -std=gnu++20 -Wall -Werror -fsyntax-only, with and without -DNDEBUG - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 44/44 passed - make unit_test DEBUG=1: 2651/2651 passed - make asan: 2651/2651 passed, 0 sanitizer reports Not run: - FP_64=1 variants (this change is FP32/FP16 only) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2d29bd9 to
4d09236
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| stored_data_size = GetSQ8StoredDataSize<VecSimMetric_L2>(params->dim, with_norm); | ||
| } else { | ||
| stored_data_size = GetSQ8StoredDataSize<VecSimMetric_IP>(params->dim, with_norm); | ||
| } |
There was a problem hiding this comment.
SQ8 element estimate skips validation
Low Severity
The new SQ8 branch in EstimateElementSize always applies GetSQ8StoredDataSize whenever quantType is VecSimQuant_SQ8, without checking that type is FLOAT32 or FLOAT16 or that the metric is supported. In the same file, NewIndex returns NULL for unsupported types and standalone Cosine, and EstimateInitialSize throws on invalid SQ8 types. Callers that size capacity from VecSimIndex_EstimateElementSize alone can get per-element byte counts for parameter sets that cannot produce an index.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 4d09236. Configure here.
There was a problem hiding this comment.
Accurate, but I do not think it should change here, and the reasoning is worth recording.
The asymmetry is pre-existing behaviour of EstimateElementSize rather than something SQ8 introduced. Its unquantized path calls VecSimParams_GetStoredDataSize (vec_utils.cpp:296-302), which is VecSimType_sizeof(type) * dim plus a Cosine/int8 adjustment and validates nothing, for any algorithm. So the function has always returned a per-element size for parameters that cannot produce an index; the SQ8 branch matches that existing contract.
Making it strict needs an error channel it does not have. The return type is size_t, so the options are a sentinel or a throw, and EstimateElementSize currently contains no throw at all: the only ones in the file are in EstimateInitialSize and the file-loading paths. Adding one would newly carry a C++ exception across the extern "C" boundary via VecSimIndex_EstimateElementSize, which this library specifically avoids, since an exception reaching the C host aborts the host process.
The genuinely inconsistent one is arguably EstimateInitialSize being strict enough to throw, not EstimateElementSize being lax. Deciding the error model for both belongs with MOD-14958, which is what first makes quantType reachable from RediSearch; there is no product exposure before then.
What I did add is coverage of the boundary that does enforce the supported set: HNSWSQ8ParamsTest.RejectsUnsupportedDataType (7719c58) asserts that FLOAT64, BFLOAT16, INT8 and UINT8 with VecSimQuant_SQ8 all return NULL from index creation, plus a comment at the estimate explaining why it deliberately does not repeat the check. Verified red without the fix: with both the type fence and the fall-through return NULL removed, all four types are silently built as unquantized indexes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1007 +/- ##
==========================================
- Coverage 97.17% 97.12% -0.06%
==========================================
Files 141 141
Lines 8328 8418 +90
==========================================
+ Hits 8093 8176 +83
- Misses 235 242 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adding `quantType` to `HNSWParams` makes it reachable on the tiered path, where nothing handles it. `TieredHNSWFactory::NewIndex` forwards `primaryIndexParams` straight into `HNSWFactory::NewIndex`, so the primary index quantizes its storage, while `NewBFParams` does not copy `quantType` and the brute-force frontend stays unquantized. The two then disagree on the stored blob layout. Reachable from any direct C API caller with `algo = VecSimAlgo_TIERED` and `quantType = VecSimQuant_SQ8`, in two ways: * FP32 / FP16: `assert(hnsw_index->getStoredDataSize() == storedDataSize)` at tiered_factory.cpp:54 aborts on a debug build. Under NDEBUG the assert is gone and the index is built with mismatched frontend and backend layouts. * FP64 / BF16 / INT8 / UINT8: `HNSWFactory::NewIndex` returns NULL for these types under SQ8, and the result is reinterpret_cast and dereferenced without a null check, so the process segfaults. The `catch (...)` in `index_factory.cpp` does not help: neither an abort nor a null dereference is an exception. RediSearch cannot set `quantType` until MOD-14958, so there is no product exposure today. This guard exists so main does not carry the defect between cherry-picks in this series. MOD-14957, which wires quantization through the tiered index properly, should replace the check and the test that covers it rather than delete them. The test builds `TieredIndexParams` with only `primaryIndexParams` set: no job queue or thread pool is needed, since the factory rejects the params before reaching anything that would use them. Deliberately not using `tieredIndexMock` here, because its destructor dereferences `ctx->index_strong_ref` unconditionally and so requires an index to have been created successfully. Verified: - Test is red without the guard and green with it: exit 134 (SIGABRT on the tiered_factory.cpp:54 assert) versus exit 0. - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 45/45 passed - make unit_test DEBUG=1: 2652/2652 passed - make asan: 2652/2652 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| } | ||
|
|
||
| VecSimIndex *NewIndex(const TieredIndexParams *params) { | ||
| // Quantization is not wired into the tiered index yet (MOD-14957). Reject it here rather than |
There was a problem hiding this comment.
Tiered estimates ignore SQ8 rejection
Medium Severity
Tiered index NewIndex functions reject invalid configurations, such as non-NONE quantization, but the associated EstimateInitialSize and EstimateElementSize functions don't perform these same validation checks. This can lead to positive memory estimates for configurations that cannot actually be instantiated.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 814eab5. Configure here.
| // type or metric cannot silently fall through and build an unquantized index instead. | ||
| return NULL; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unknown quantType builds unquantized index
Medium Severity
HNSWFactory::NewIndex only handles quantType == VecSimQuant_SQ8 explicitly. Any other non-VecSimQuant_NONE value falls through to the ordinary unquantized CreateIndexComponents path and builds a full-precision HNSW index without error. Callers that set an unsupported or forward-looking quantization code get silent misconfiguration instead of NULL, unlike the tiered factory which rejects any non-NONE quantType.
Reviewed by Cursor Bugbot for commit 814eab5. Configure here.
| est += allocations_overhead + sizeof(MultiPreprocessorsContainer<float, 1>); | ||
| est += allocations_overhead + sizeof(QuantPreprocessor<float, VecSimMetric_L2>); | ||
| } | ||
| est += EstimateInitialSize_ChooseMultiOrSingle<float>(params->multi); |
There was a problem hiding this comment.
SQ8 initial size skips metric
Low Severity
The VecSimQuant_SQ8 branch in EstimateInitialSize validates params->type but not params->metric. Configurations such as SQ8 with VecSimMetric_Cosine receive a full SQ8 initial-size estimate even though NewIndex returns NULL for the same params (cosine is rejected unless remapped via is_normalized, which the public C API does not use).
Reviewed by Cursor Bugbot for commit 814eab5. Configure here.
SQ8 quantizes to uint8 with FP32 metadata and only has kernels for FP32 and FP16 sources, so every other data type must be rejected at index creation. Nothing covered that, which Cursor Bugbot noticed from the other direction on #1007: it flagged that `EstimateElementSize` will happily size a configuration that `NewIndex` refuses to build. That asymmetry is intentional and pre-existing rather than something SQ8 introduced. `EstimateElementSize`'s unquantized path calls `VecSimParams_GetStoredDataSize` (vec_utils.cpp:296), which is `VecSimType_sizeof(type) * dim` plus a Cosine adjustment and validates nothing for any algorithm, so the function has always answered for parameters that cannot produce an index. Making it strict would mean either inventing a sentinel for a `size_t` return or throwing, and `EstimateElementSize` currently contains no `throw` at all, so that would newly carry a C++ exception across the `extern "C"` boundary through `VecSimIndex_EstimateElementSize`. Settling the error model for these two functions belongs with MOD-14958, which is what first makes `quantType` reachable from RediSearch. So this pins the boundary that actually enforces the supported set, and records in a comment why the estimate deliberately does not repeat it. Verified: - Test is red without the fix: removing both the type fence and the fall-through `return NULL` makes it fail for all four types (FLOAT64, BFLOAT16, INT8, UINT8), which are otherwise silently built as unquantized indexes. - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 46/46 passed - make unit_test DEBUG=1: 2653/2653 passed - make asan: 2653/2653 passed, 0 sanitizer reports (the new test exercises the early-return path, so this also covers leaking the allocator set up before it) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lerman25
left a comment
lerman25
left a comment
There was a problem hiding this comment.
I lack context for this,
Left some comments, some are AI that seem reasonable
Also there are other AI comments if you can address them
|
|
||
| VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { | ||
| const HNSWParams *hnswParams = ¶ms->algoParams.hnswParams; | ||
|
|
There was a problem hiding this comment.
Will take it, thanks. Cosmetic only, so I have grouped it with the assert suggestion below rather than pushing a commit for a blank line on its own.
|
|
||
| // Unreachable today: the checks above leave only FP32/FP16 x L2/IP. Kept so that adding a | ||
| // type or metric cannot silently fall through and build an unquantized index instead. | ||
| return NULL; |
There was a problem hiding this comment.
Maybe assert false here ?
There was a problem hiding this comment.
Good call, and it matches repo precedent (svs_factory.cpp:89 and friends use assert(false && "...") for unreachable type/metric combinations).
I would like to do both rather than swap one for the other:
assert(false && "unhandled SQ8 type/metric combination");
return NULL;The assert makes a debug build shout if a future type or metric reaches here, which is what you are after. Keeping the return NULL means a release build still fails closed instead of falling through and silently building an unquantized index, which is the regression that line exists to prevent (it was missing until 4d09236). Assert-only would restore exactly that hole under NDEBUG.
Shout if you would rather have the assert alone and I will drop the return.
There was a problem hiding this comment.
Done in 9e69259, kept alongside the return NULL as described above.
| ASSERT_EQ(GenerateAndAddVector(0, 0.25f, 0.25f), 1); | ||
| data_t query[4]; | ||
| GenerateVector(query, 0.5f, 0.25f); | ||
| auto processed_query = CastToHNSW()->preprocessQuery(query); |
There was a problem hiding this comment.
This test bypasses the public VecSimIndex_GetDistanceFrom_Unsafe contract by manually constructing an internal processed query. The API documents blob as a raw type×dimension vector, and C callers have no preprocessing API. For FP32 dim=4 L2, a valid raw query is 16 bytes, but the SQ8 kernel reads the appended y_sum and y_sum_squares at bytes 16–23, causing out-of-bounds reads. Please test this API with query directly and either preprocess internally, expose a public reusable prepared-query context, or reject direct-distance lookups for SQ8.
There was a problem hiding this comment.
Confirmed and fixed in 63271c1. You were right, and it is worse than a contract mismatch: it is an out-of-bounds read reachable from the public C API. I reproduced it under AddressSanitizer with a dim=4 FP32 L2 SQ8 index and a correctly sized 16-byte heap query:
ERROR: AddressSanitizer: heap-buffer-overflow, READ of size 4
#0 SQ8_FP32_InnerProduct_Impl IP.cpp:65
#6 VecSimIndex_GetDistanceFrom_Unsafe vec_sim.cpp:231
Your diagnosis of why the suite missed it is also exactly right: the test obtained a preprocessed blob through CastToHNSW()->preprocessQuery(...), a C++-only path no C caller has.
getDistanceFrom_Unsafe now returns INVALID_SCORE for a quantized index, which is already the value getDistanceFromInternal uses for "no answer", so it needs no new error channel. I considered your first suggestion, preprocessing internally, and did not take it here: preprocessQuery also normalizes cosine queries, so applying it would change behaviour for every existing cosine index, and it would add a per-call allocation on RediSearch's scoring path. Your third suggestion, a public reusable prepared-query context, is the right long-term answer and belongs with MOD-14958, which is what first exposes any of this to the host.
The test now checks the distance maths through calcDistanceForQuery and separately asserts the public API reports no answer for a raw vector, so the raw-blob call is exercised under ASan by every type parameter.
| mean_sum_squares += v * v; | ||
| } | ||
|
|
||
| pp = new (allocator) QuantPreprocessor<DataType, Metric, true>(allocator, dim, mean_vec); |
There was a problem hiding this comment.
Blocking: FP16 + mean + L2 loses correctness through this instantiation. QuantPreprocessor<float16, L2, true>::preprocessQuery computes input[i] - mean[i] in FP32, then narrows it back into the FP16 query body, while storage quantization keeps its centered min/delta in FP32. Identical vector/query pairs can therefore diverge: for x = y = [1,1,1,1] and mean [10000,...], storage represents -9999 but the query rounds to -10000, yielding self-distance 4. A valid FP16 query -40000 with mean 40000 also overflows after centering. Please keep mean-centered FP16 L2 queries in FP32 with a matching asymmetric kernel, or reject/validate this combination, and add a regression.
There was a problem hiding this comment.
Confirmed and fixed in 63271c1. I reproduced your numbers exactly using the repo's own conversions before changing anything:
x = 1, mean = 10000
centred storage (fp32) = -9999.0
centred query (fp16) = -10000.0 -> per-component error 1.0
L2^2 for an identical vector/query pair at dim=4 = 4.0
centring -40000 with mean 40000 = -80000 -> fp16 -inf
One qualifier worth recording: at a realistic mean near 1 the error is exactly zero, so this only bites for large mean magnitudes. It is silent when it does, though, so it still needs handling.
HNSWFactory::NewIndex now rejects FLOAT16 + mean + L2. Note the scope is narrower than the comment implies: only the WithNorm && L2 branch centres the query, so FP16 + mean + IP is unaffected and stays supported. RejectsMeanCenteredFP16L2 pins both halves of that.
Your preferred fix, keeping the centred query in FP32 with a matching asymmetric kernel, is the correct one but it is an ARM design change plus new kernel work, so I have left it for their series rather than doing it in a cherry-pick. FLOAT16-with-mean also leaves the functional type set, since every functional test uses L2; that trades 11 typed tests for correctness, and none of them were exercising a combination that still works.
| abstractInitParams.storedDataSize = GetSQ8StoredDataSize<Metric>(dim, with_norm); | ||
|
|
||
| // Symmetric: both stored vectors are SQ8 blobs. | ||
| auto sym_func = spaces::GetDistFunc<sq8, float>(Metric, dim, &storage_alignment); |
There was a problem hiding this comment.
Blocking: the newly selected symmetric SQ8 kernel can overflow for valid large dimensions. On AVX512 VNNI, SQ8_SQ8_InnerProductImp receives an int from UINT8_InnerProductImp, whose horizontal reduction is signed 32-bit. A dimension-33027 vector that quantizes one component to 0 and 33026 components to 255 has self-dot 65025 * 33026 = 2147515650, exceeding INT_MAX. The wrapped value feeds both IP and L2 graph construction/pruning, so HNSW can be built with incorrect distances. Please use a wide/chunked accumulation or select a safe fallback above the overflow boundary, and add a boundary regression.
There was a problem hiding this comment.
The mechanism is real and I verified it, but I would like to take it as a separate ticket rather than in this cherry-pick. Two corrections to the scope first, both of which make it worth its own change:
It is not AVX512-only. All four SIMD SQ8-to-SQ8 kernels route through UINT8_InnerProductImp and inherit the int accumulator: IP_AVX512F_BW_VL_VNNI_SQ8_SQ8.h, IP_NEON_SQ8_SQ8.h, IP_NEON_DOTPROD_SQ8_SQ8.h and IP_SVE_SQ8_SQ8.h. The scalar fallback in IP.cpp:150 accumulates into a float and does not wrap, so the exposure is exactly the SIMD paths, on x86 and ARM alike.
The threshold is confirmed: the per-element product caps at 255*255 = 65025, and _mm512_reduce_add_epi32 returns int, so the sum exceeds INT_MAX from dim = 33026 (your 33026 case overshoots by 32,003). I found no dimension cap anywhere in VecSim, so it is reachable in principle from a direct C API caller.
My reasoning for separating it: the kernels are pre-existing (they landed with the SQ8 distance-function PRs and are already exercised by test_spaces and the benchmarks), this PR is only the first thing to select them for storage-to-storage comparisons, and the trigger needs a dimension over 33025 plus data that quantizes almost every component to 255. Fixing it properly means widening or chunking the accumulation in the shared UINT8 helper, which changes int8/uint8 index behaviour too and deserves its own boundary regression rather than riding along here.
I will open a ticket against the kernels with the above. Say the word if you would rather it block this PR and I will pull it in.
| unsigned char storage_alignment = 0, asym_storage_alignment = 0; | ||
|
|
||
| // Override blob size for the SQ8 storage layout. | ||
| abstractInitParams.storedDataSize = GetSQ8StoredDataSize<Metric>(dim, with_norm); |
There was a problem hiding this comment.
The existing test serializer now silently accepts a layout its loader cannot decode. V4 records type, dim, and metric, but not quantType or the mean, and loading always constructs unquantized components. For example, FP32/L2 at dim 128 writes a 144-byte SQ8 blob per vector, while the loader expects 512 bytes of FP32 data and consumes following graph bytes as vector data. If SQ8 serialization is intentionally deferred, please make saveIndex() reject SQ8 and test that failure so it cannot emit a corrupt/unloadable file.
There was a problem hiding this comment.
Confirmed structurally, and I agree with your proposed fix. quantType appears nowhere in hnsw_serializer.h, and the file-loading path in HNSWFactory::NewIndex always builds components through CreateIndexComponents, which has no SQ8 branch at all, so a saved SQ8 index reloads as an unquantized one with the wrong stride.
This is the same shape as the tiered hazard: a combination that is not wired yet but is silently accepted. The guard belongs in this PR by the same argument, and the isQuantized flag added in 63271c1 for the distance-API fix is what saveIndex would test.
I have not done it in this round because the requested scope was the two blocking findings. It is a small follow-up: reject SQ8 in saveIndex and test that it fails rather than emitting a file the loader cannot decode. Happy to add it here if you want it before merge.
There was a problem hiding this comment.
Done in 9e69259. saveIndexIMP now throws for a quantized index, covered by HNSWSQ8Test.RejectsSerialization across all three type parameters.
One wart to flag rather than hide: HNSWSerializer::saveIndex writes the encoding version before calling saveIndexIMP, so a rejected save leaves a stub file behind. That still fails closed on load, unlike a complete file whose layout the loader misreads, but validating before the file is created would need a new virtual hook on the serializer base across four files, which felt disproportionate for a path that cannot be reached from RediSearch yet. Noted in the carry-forward doc so whoever adds real SQ8 serialization moves the check earlier.
| static constexpr bool with_quant_params = WithQuantParams; | ||
| }; | ||
|
|
||
| using HNSWSQ8DataTypeSet = |
There was a problem hiding this comment.
This type set varies source type and mean presence, but not metric or multi. As a result, the graph/search/range/batch/override tests all exercise the default L2 single-index path; IP only appears in the one-vector direct-distance test. That leaves the new symmetric IP kernel used during HNSW graph construction and the multi-label path untested. Please parameterize the functional suite over metric and multi, and use non-constant vectors so those paths are meaningfully exercised.
There was a problem hiding this comment.
Accurate, and sharper than the note I had written for myself. Confirming the specifics: the type set varies only source type and mean presence, every functional test takes the default L2 path, and IP appears solely in the one-vector distance test, so the symmetric SQ8-to-SQ8 IP kernel used during graph construction is genuinely unexercised. Your point about constant vectors is right too: GenerateVector defaults to step = 0, so most tests build all-equal components and take the degenerate min == max quantization branch.
Two notes on the current state. 63271c1 removed FLOAT16-with-mean from that type set, because every functional test uses L2 and mean-centred FP16 L2 is now rejected, so FP16 + mean + IP is currently left with construction coverage only. That makes the metric axis you are asking for more valuable, not less.
I have not parameterized over metric and multi in this round, since the requested scope was the two blocking findings and this is ARM's suite. It is the right next step and I would rather do it deliberately than bolt it on: the L2 expectations in several tests are hard-coded, so adding the metric axis means reworking the expected values, not just widening the type list. Tell me if you want that in this PR or tracked for the series.
There was a problem hiding this comment.
Partly addressed in 9e69259, and you were right that it mattered.
GraphConstructionIP builds a 100-vector dim-16 IP index and searches it, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects now actually runs. Its vectors also vary per component rather than only per label, so it does not take the degenerate min == max branch that the existing tests all hit.
Worth reporting how that went, since it supports your point: my first version asserted that querying with a copy of an inserted vector would return that label first. It failed, returning 99 instead of 70, because this is plain inner product rather than cosine: the distance is 1 - IP, so the winner is the vector with the largest projection, not the query's own twin. The expectation now derives from the metric instead of from assumed self-similarity. An untested kernel plus an untested assumption is exactly the gap you were pointing at.
Not done: full parameterization over metric and multi. Several existing tests hard-code L2 expectations, so that axis means reworking their expected values rather than widening the type list, and it is ARM's suite. I would rather track it for the series than half-do it here. The multi-label path is still uncovered.
Both were raised by @lerman25 and both are real. Verified before fixing rather than taken at face value. 1. Out-of-bounds read through the public C API --------------------------------------------- `VecSimIndex_GetDistanceFrom_Unsafe` documents `blob` as a raw vector matching the index data type and dimension. For a quantized index that is not a usable query blob: `QuantPreprocessor::preprocessQuery` appends FP32 query metadata (`y_sum`, and `y_sum_squares` for L2) which the SQ8 kernels then read, so honouring the documented contract reads past the caller's buffer. Reproduced with AddressSanitizer on a dim=4 FP32 L2 SQ8 index and a correctly sized 16-byte heap query: ERROR: AddressSanitizer: heap-buffer-overflow, READ of size 4 #0 SQ8_FP32_InnerProduct_Impl IP.cpp:65 #6 VecSimIndex_GetDistanceFrom_Unsafe vec_sim.cpp:231 `getDistanceFrom_Unsafe` now returns `INVALID_SCORE` for a quantized index, which is the value `getDistanceFromInternal` already uses for "no answer", so this needs no new error channel. Preprocessing internally was rejected as the fix here: `preprocessQuery` also normalizes cosine queries, so applying it would change behaviour for every existing cosine index, and it would add a per-call allocation on RediSearch's scoring path. A public prepared-query API is the real answer and belongs with MOD-14958. `AbstractIndexInitParams` gains `isQuantized` for this, parallel to `isDisk`. It defaults to false, so every other factory is unaffected, and the same flag is what a serialization guard would need. 2. Mean-centred FP16 L2 loses correctness ----------------------------------------- `QuantPreprocessor<float16, L2, true>::preprocessQuery` centres the query then narrows the result back into the FP16 query body, while storage keeps its centred min/delta in FP32. The two disagree. Verified numerically with the repo's own conversions: x = 1, mean = 10000 centred storage (fp32) = -9999.0 centred query (fp16) = -10000.0 -> per-component error 1.0 L2^2 for an identical vector/query pair at dim=4 = 4.0 centring -40000 with mean 40000 = -80000 -> fp16 -inf At a realistic mean near 1 the error is exactly zero, so this only bites for large mean magnitudes, but it is silent when it does. `HNSWFactory::NewIndex` now rejects FLOAT16 + mean + L2. The same combination with IP is unaffected and still supported, because that path does not centre the query. Fixing it properly means keeping the centred query in FP32 with a matching asymmetric kernel, which is ARM's design and belongs upstream. Test changes ------------ `test_get_distance` verified the distance maths through `VecSimIndex_GetDistanceFrom_Unsafe`, but passed it an internally preprocessed blob obtained via a C++-only path no C caller has, which is why the suite missed the overflow. It now checks the maths through `calcDistanceForQuery` and separately asserts that the public API reports no answer for a raw vector. That call is exercised under ASan by every type parameter. FLOAT16 with a mean vector leaves the functional type set, because every functional test uses L2 and that combination is now rejected. It is covered explicitly by `RejectsMeanCenteredFP16L2`, which also pins that FP16 + mean + IP still constructs. Net effect on the suite is 2653 -> 2643 tests: the 11 dropped typed tests were all exercising a combination that is now unsupported, so nothing that previously worked lost coverage. FP16 + mean + IP is left with construction coverage only and no functional search coverage, which is worth closing alongside the metric/multi parameterization also raised in review. Verified: - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 36/36 passed - test_hnsw_sq8 under ASan: 36/36, 0 sanitizer reports (the ASan repro above is clean after the fix) - make unit_test DEBUG=1: 2643/2643 passed - make asan: 2643/2643 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 63271c1. Configure here.
Remaining review points from #1007, other than the dim >= 33026 kernel overflow which is recorded in SQ8-SERIES-CARRYFORWARD.md instead. Serialization ------------- The V4 format records type, dim and metric, but neither quantType nor the mean vector, and the file-loading path in HNSWFactory always builds components through CreateIndexComponents, which has no SQ8 branch. A saved SQ8 index therefore reloads as unquantized over quantized bytes, misreading the stride and consuming graph bytes as vector data. saveIndexIMP now throws for a quantized index. This is the same argument as the tiered guard: the combination is not wired yet, so fail closed rather than accept it silently. One wart worth knowing: the caller writes the encoding version before saveIndexIMP runs, so a rejected save leaves a stub file. That still fails closed on load, unlike a complete file with a layout the loader misreads, but whoever adds real SQ8 serialization should move the check ahead of the file being created. Recorded in the carry-forward file, whose "serializer should refuse to save" item this closes. IP graph construction --------------------- Every other functional test uses L2, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects for an IP index was never executed. That kernel is pre-existing, but this series is the first thing to put it on the insert path, so it should not go in untested. GraphConstructionIP builds a 100-vector dim-16 IP index and searches it. The expected result follows from the metric rather than from assumed self-similarity: this is plain inner product, not cosine, so the distance is 1 - IP and the closest vector is the one with the largest projection onto the query. Vectors and query are positive with magnitude growing by label, so results come back from the highest label downward. My first version of this test asserted the query's own label would rank first and failed correctly, returning 99 instead of 70. Vectors also vary per component, not just per label, so quantization does not collapse into the degenerate min == max branch that the existing tests all take. Review nits ----------- * assert(false && "...") added before the unreachable return NULL in the SQ8 branch, matching svs_factory.cpp. Kept alongside the return rather than replacing it: assert-only would reopen the silent-unquantized-fallthrough hole under NDEBUG, which is the regression that line exists to prevent. * Dropped the blank line this series added after the hnswParams declaration. Verified: - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 42/42 passed - make unit_test DEBUG=1: 2649/2649 passed - make asan: 2649/2649 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Describe the changes in the pull request
Cherry-pick of ARM-software/VectorSimilarity-for-Arm#4 (head
125ea15d), plus a follow-up review pass. Fourth PR in the SQ8 series, after #999, #1000 and #1002.Adds 8-bit scalar quantization (SQ8) to the standalone HNSW index:
VecSimQuantTypeplusquantType/quantParamsonHNSWParams. Both fields are appended at the end of the struct andVecSimQuant_NONEis 0, so existing zero-initialized and designated-initializer construction is unaffected.HNSWFactorycan build SQ8 indexes for FLOAT32 and FLOAT16 data types with the L2 and IP metrics, wiringQuantPreprocessorandDistanceCalculatorWithNorm, and accounts for SQ8 inEstimateInitialSizeandEstimateElementSize.quantParamspoints to afloat[dim]mean vector; a null pointer selects quantization without mean normalization.test_hnsw_sq8unit-test target and suite (44 tests: FP32/FP16 x L2/IP).SQ8 support for the tiered HNSW index, serialization and benchmarks is deferred to later PRs in the series.
Commit 1: the cherry-pick
ARM's four commits squashed into one, with no functional change to their code. Two Redis-side adjustments:
SPDX-FileCopyrightTextArm line from the two modified files, matching how [MOD-14953] Add calcDistanceForQuery to IndexCalculatorInterface #999, [MOD-14952] Support normalization in QuantPreprocessor #1000 and [MOD-14955] Add DistanceCalculatorWithNorm #1002 landed. It is kept on the newtests/unit/test_hnsw_sq8.cpp, where theBSD-3-Clauseidentifier was replaced by this repo's Redis tri-license header.make check-formatpasses at the 100-column limit.Commit 2: review follow-up
No behavioural change intended. Interface, single-source-of-truth and idiom fixes:
quantParamsis nowconst void *. Every use reads it and two already cast toconst float *. Layout-identical, so not an ABI break, and callers passing non-const still compile. Better to fix before the field ships and freezes.VecSimQuant_SQ8comment claimed "with mean normalization", but mean normalization is optional and selected byquantParams. Reworded.GetSQ8StoredDataSizere-derived the stored blob size thatQuantPreprocessor's constructors already computed. Two formulas for one layout drift silently, which is the bug class fixed in MOD-15303. The formula now lives once assq8::storage_bytes_count<Metric, WithNorm>(dim), beside thestorage_metadata_countit builds on, and both callers use it. This is why the diff touchestypes/sq8.handspaces/computer/preprocessors.h, two files beyond ARM's original four: having one shared definition is the entire point of the fix.return NULLclosing the SQ8 branch. Unreachable today, since the type and metric checks leave only FP32/FP16 x L2/IP, but without it adding a type or metric silently falls through and builds an unquantized index.assert(ret == 0)onaddPreprocessoris nowassert(ret != -1). The function returns -1 on failure, 0 when the container is full, and the next free index otherwise, so 0 is merely the only success value at the current container size of one.!= -1is the documented contract and the existing repo idiom.addPreprocessor, assert,IndexComponents, return); only the preprocessor and calculator differ.assigninstead of a zero-filling constructor followed bymemcpy, which wrote every element twice.GetDistFuncfor a function that is never used, sincespaces.hoffers no alignment-only query and the asymmetric hint covers the storage operand. That call now lives in a smallGetQueryAlignment<DataType>adapter returning the hint, so the call site neither discards a value nor keeps a third distance function in scope besidesym_funcandasym_functhat must never be called.query_alignmentis const.GetSQ8StoredDataSizeis[[nodiscard]] constexpranddim/with_normare const.Commit 3: reject quantized tiered indexes until MOD-14957
Adding
quantTypetoHNSWParamsmakes it reachable on the tiered path, where nothing handles it.TieredHNSWFactory::NewIndexforwardsprimaryIndexParamsintoHNSWFactory::NewIndex, so the primary index quantizes its storage, whileNewBFParamsdoes not copyquantTypeand the frontend stays unquantized. Reachable from any direct C API caller withalgo = VecSimAlgo_TIEREDandquantType = VecSimQuant_SQ8:getStoredDataSize()assert attiered_factory.cpp:54aborts on a debug build; underNDEBUGthe index is built with mismatched frontend and backend layouts.HNSWFactory::NewIndexreturns NULL for these types under SQ8, and the result isreinterpret_castand dereferenced with no null check, so the process segfaults.The
catch (...)inindex_factory.cppdoes not help, since neither an abort nor a null dereference is an exception. RediSearch cannot setquantTypeuntil MOD-14958, so there is no product exposure today; the guard exists so main does not carry the defect between cherry-picks. MOD-14957 should replace this check and its test rather than delete them.Commit 4: cover SQ8 rejection of unsupported data types
HNSWSQ8ParamsTest.RejectsUnsupportedDataTypeasserts that FLOAT64, BFLOAT16, INT8 and UINT8 withVecSimQuant_SQ8all returnNULLfrom index creation. Verified red without the fix: with both the type fence and the fall-throughreturn NULLremoved, all four are silently built as unquantized indexes. Also documents atEstimateElementSizewhy the estimate deliberately does not repeat the check (see the Bugbot thread on this PR).Commit 5: fix two defects found in review (63271c1)
Both raised by @lerman25, both verified before fixing.
VecSimIndex_GetDistanceFrom_Unsafedocumentsblobas a raw dim-by-type vector, but the SQ8 kernels read query metadata appended past that, so honouring the documented contract read past the caller's buffer. Reproduced under ASan:heap-buffer-overflow, READ of size 4inSQ8_FP32_InnerProduct_Implviavec_sim.cpp:231.getDistanceFrom_Unsafenow returnsINVALID_SCOREfor a quantized index, the valuegetDistanceFromInternalalready uses for "no answer".AbstractIndexInitParamsgainsisQuantizedfor this, parallel toisDisk. Preprocessing internally was rejected becausepreprocessQueryalso normalizes cosine queries, so it would change behaviour for every existing cosine index; a public prepared-query API is the real answer and belongs with MOD-14958.-inf. Now rejected at construction. FP16 + mean + IP is unaffected and still supported, since that path does not centre the query.The test that should have caught the first one passed a preprocessed blob obtained through a C++-only path no C caller has. It now checks the maths via
calcDistanceForQueryand separately asserts the public API reports no answer for a raw vector.Commit 6: serialization guard and IP graph coverage (9e69259)
saveIndexIMPrefuses a quantized index. The V4 format records neitherquantTypenor the mean, and the loading path always builds unquantized components, so a saved SQ8 index reloaded with the wrong stride and consumed graph bytes as vector data. Same argument as the tiered guard: fail closed. Caveat: the encoding version is written before this check runs, so a rejected save leaves a stub file, which still fails closed on load.GraphConstructionIP. Every other functional test uses L2, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects was never executed, and this series is the first thing to put it on the insert path. Its vectors vary per component, so it avoids the degeneratemin == maxbranch the other tests take.assert(false && "...")before the unreachablereturn NULL(kept alongside it, since assert-only would reopen the silent-fallthrough hole underNDEBUG), and the stray blank line removed.Verification
Run on the final tree, after both commits:
./check-format.shg++ -Wall -Werror -fsyntax-only, with and without-DNDEBUGmake build DEBUG=1test_hnsw_sq8make unit_test DEBUG=1make asanNot run:
FP_64=1variants, since this change is FP32/FP16 only.Both new guards were confirmed red without their fix, and the ASan repro above is clean after it.
Test count moved 2653 -> 2649. Dropping FLOAT16-with-mean from the functional type set removed 11 typed tests for a combination that is now rejected, and the two new tests added 6 back. FP16 + mean + IP is left with construction coverage only, and the multi-label path and full metric parameterization remain uncovered; both are tracked rather than silently dropped.
Reviewed and deliberately left alone
query_alignmenthint comes from the symmetricDataTypedispatcher while the asymmetric kernel that consumes the query uses unaligned loads (_mm512_loadu_ps). This costs nothing:QuantPreprocessor::preprocessQueryalways allocates a fresh blob viaallocate_aligned, so the hint only selects that allocation's alignment. It matches the asymmetric-types contract inspaces.h.EstimateInitialSizeuses<float>for the index class even on the FP16 path. Verified correct with astatic_assertonsizeoffor both the single and multi index classes.mean_sum_squaresaccumulates infloat. It is a constant additive term on the IP path only, identical for every candidate, so it cannot affect ranking, only the absolute reported distance. Left as is.new (allocator)calls with no RAII between them leak if a later constructor throws.preprocessors_factory.hdoes the same, so this is repo-wide debt rather than something this PR introduced.Which issues this PR fixes
Main objects this PR modified
HNSWFactoryindex creation and memory estimationHNSWParamsand the newVecSimQuantTypepublic APIsq8::storage_bytes_count, now the single definition of the SQ8 storage layout sizeMark if applicable
🤖 Generated with Claude Code
Note
Medium Risk
New public HNSW params and quantized distance/graph paths affect core index behavior; guards limit tiered/serialization/raw-distance footguns, but wrong metric/type handling would silently mis-rank or corrupt memory if validation regresses.
Overview
Adds 8-bit scalar quantization (SQ8) to standalone HNSW via new
VecSimQuantType/quantTypeand optionalquantParams(FP32 mean) onHNSWParams, plus anisQuantizedflag on index init.HNSWFactory builds SQ8 indexes for FP32/FP16 with L2 or IP (optional mean normalization), wiring
QuantPreprocessorand SQ8 distance calculators; memory estimates usesq8::storage_bytes_countas the single layout-size definition. Unsupported combos fail closed (cosine, other dtypes, mean-centered FP16+L2).Safety guards: quantized indexes refuse save (format cannot round-trip), return
INVALID_SCOREfromgetDistanceFrom_Unsafeon raw query blobs, and the tiered factory rejects any non-NONEquantTypeuntil tiered SQ8 exists.Adds
test_hnsw_sq8covering creation, search, sizing, serialization rejection, and param validation.Reviewed by Cursor Bugbot for commit 9e69259. Bugbot is set up for automated code reviews on this repo. Configure here.